Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#110 Group Unicode symbols into browsable categories #353

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

ElliotRoe
Copy link

@ElliotRoe ElliotRoe commented Jan 23, 2024

Draft PR for issue #110

Changes

  • Created widgets label and dropdown menu that mirrored the Figma prototype
  • Updated Glyphchooser bar to match the design proposal
  • Upon selecting a category from the dropdown, the search area is expanded and filtered by category
  • Glyphs are displayed with a virtual list so as not to stress rendering when there are large unfiltered results to be displayed
  • Glyph search now is powered by fuzzysort which fuzzy matches search terms to glyph names and allows for a larger quantity of relevant glyphs to be discovered
    • Could also be helpful in the future for multi-lingual searches using their weighting feature
    • Created a wordplay category config file at src/unicode/wordplay-categories.json. However, I noticed that there was already an emoji specification in the Unicode file that seemed to be a superset of the one given in the proposal. I had wordplay default to this category instead of the smaller proposed category.

Questions

  • I found myself creating a lot of wrapping divs to apply styles to widgets. Not sure if this is the correct pattern to use, I have not used pure CSS with Svelte before. Please let me know if there is a better way to do this
  • What is the scope of recently used glyphs? Are they specific to a project? Or to the user overall? What is the best way to add this piece of metadata to Firebase? Is there a specific interface I should add the field to?
  • I saw that the value calc(var(--wordplay-spacing)/2) was being used in multiple areas. I also included it in my label and dropdown design. It is useful as var(--wordplay-spacing) can be too large for some inner padding of components. Should this be refactored into it's own separate CSS variable?

Known Issues

  • Due to the high volume of rendering, when scrolling through the "other" category
  • Hacky solution for including recently used bar into the scrollable virtual list

To Do

The following are the remaining deliverables in the scope of this ticket (I believe):

Round 1 Feedback

@amyjko
Copy link
Collaborator

amyjko commented Jan 26, 2024

Thanks @ElliotRoe! I should have time to review this weekend.

One quick thing now: make sure there are no package-lock.json updates in your branch; use npm ci to get package updates, not npm update.

@amyjko amyjko marked this pull request as ready for review January 28, 2024 17:25
@amyjko amyjko marked this pull request as draft January 28, 2024 17:25
Copy link
Collaborator

@amyjko amyjko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great start! Thank you for taking on this relatively hefty issue.

  • I'm always a bit loathe to take on additional dependencies unless we're quite sure that they 1) have zero to few dependencies themselves, 2) don't have lurking accessibility and localization flaws, and 3) are actively maintained. I did a cursory review of fuzzysort and virtual list and they seem to meet these criteria relatively well, but I think there's some due diligence in order, especially on item 2.
  • The PR has some TypeScript and Svelte errors and warnings. Run npm run check and you'll see some accessibility warnings that need to be resolved and some missing type declarations.
  • All of the labels need to be localized; that means declaring new strings in UITexts.ts and creating placeholders in all supported locales, including en-US, zh-CN, and es-MX, and retrieving strings from the active locale. You'll see other patterns that use the $locale store defined in Database.ts
  • Wrapping divs are fine for styling; the Svelte way is the HTML/CSS way; there are no special toolkit specific patterns, just the patterns we're using in this project.
  • Creating a smaller spacing variable is probably a good idea, given how much we're duplicating that pattern. But you're welcome to open a separate issue for that, since it's out of scope for this issue.
  • For recently used glyphs, I think the best place to store recently used glyphs would be at the project level, since that's where recency is likely to be a good indicator of relevance. Another option, which is a slight deviation from the design, is to extract them from the project itself, and the undo history, rather than persisting a history. I think it would be fine to scope it down to that for now, and potentially persist it at the project level as a later enhancement.
  • The vertical alignment of the search box should probably be baseline aligned rather than centered, since it's hard to tell that it is searching the view.
  • The hit area for emojis should probably be larger, with reduced padding between. They're hard to tap and click.
  • The background on the "Operators" and "Recently Used" labels breaks convention; no other parts of the interface have shading behind a header label. Go ahead and remove that.
  • The group chooser isn't not keyboard accessible. Space activates the pop menu, but there is no way to change the group after that. The control should be WCAG 2.2 compliant.
  • Typing in the search box grows the search box but this shrinks the search results box. I suggest not growing the search box, keeping the layout fixed.
  • It's not clear what the recently used values are currently coming from; maybe they're just placeholders?
  • There should be feedback when the search results are empty. Without this, when changing to the category, the results are often empty, since nothing matches the search.
  • Definitely need to resolve the performance issues with the other category.
  • Using the emoji specification in unicode instead of the proposed category is fine; I think the designers didn't quite capture everything.

Thanks for the great start on this!

@ElliotRoe
Copy link
Author

Thanks so much for your detailed feedback! I will keep working on the rest of the deliverables for the issue and implement your feedback. I would expect to be in a good place for another round of feedback in about two weeks. Thanks!

@ElliotRoe ElliotRoe linked an issue Feb 12, 2024 that may be closed by this pull request
@ElliotRoe
Copy link
Author

Hi @amyjko. Just following up with a quick update and a clarifying question about your feedback regarding search bar alignment. I'm behind where I thought I would be two weeks ago, but have continued to make progress on the issue which I'm tracking above in the body of the PR.

In regards to your comment, "The vertical alignment of the search box should probably be baseline aligned rather than centered, since it's hard to tell that it is searching the view.", could you modify the original design Figma or give a quick sketch of what the alignment should look like? I'm not sure what you mean by this. I understand baseline is a CSS option for flex box item aligning, but changing the alignment from center to baseline puts the bar at the very bottom of the view which I'm not sure is what you intended.

I'll check in again in around two weeks! I can't promise that the PR will be in a good place for another round of feedback then, as my spare dev time is decreasing with the semester ramping up but I'll continue to chip away at the issue and track my progress above. I've been having tons of fun coding so far!

Thanks!

@amyjko
Copy link
Collaborator

amyjko commented Feb 16, 2024

Thanks for the update @ElliotRoe!

I don't have access to the original Figma sketches to modify them, so I'll try to clarify here with words. Visually, I think vertically aligning the search field at the top of the container (with the recently used row), rather than aligned with the top of the emoji grid, would make more sense semantically, because the default operators and recently used emojis disappear upon search. If the default operators and recently used were persistent, even after search, then it might make more sense to vertically align it with just the grid. When I said "baseline", I was more referring to how to vertically align: ideally, the search box's baseline and the row of recently used emojis would be vertically aligned, at the top of the container.

I can add a new sketch if this isn't clear.

@ElliotRoe
Copy link
Author

Gotcha, that clears it up. Thanks!

@ElliotRoe
Copy link
Author

Hi! Just checking in with a quick update. I was able to chip away at most of the trivial tasks for the ticket. With the help of my wonderful labmate Grace Barkhuff, I've begun implementing a WCAG 2.2 compliant combobox for the drop-down menu. Additionally, I have also started on modifying the necessary JSON project schemas to enable the recently used feature for Firebase. I hope to have these larger features of the ticket finished by mid April. Thanks! -Elliot

@ElliotRoe
Copy link
Author

Pinging here that this ticket is still on my radar! More commits will roll in this coming weekend. Thanks.

@amyjko
Copy link
Collaborator

amyjko commented Apr 8, 2024

Excellent, we look forward to it!

Deleted lock file and completed synced with upstream lock file. Added on click toggle listener
@ElliotRoe
Copy link
Author

Hello @amyjko! Combobox ui component and recently used feature have been completed. The last larger task that I need to complete is the fixing of the "other" category scrolling issues then I'll just have some final clean up and localization tasks. I'll quickly go over my recently used implementation as it has the largest design consequences that you may want to weight in on.

As you suggested, I implemented the recently used glyphs feature at the project level. I added a new array of strings as a property to the projectSchemaV2 zod schema, then modified the appropriate helper functions to account for the new property. I had two questions:

  1. Will we need to version the project schemas? Right now, I just have it to default to an empty array if no recentlyUsed property is defined on the project object. Is this acceptable?
  2. I wasn't totally sure where the best place to house the recently used array concatenation logic. For now it's in the GlyphSearchArea component, but I noticed a lot of event logic being handled in ProjectView.svelte. I tried to do something similar to the pattern that I saw there with svelte's event dispatcher api but for some reason it was not playing nice with the editor.edit call. I also considered handling it there, but wasn't sure if that made total sense either. Any thoughts or suggestions?

Thanks, shooting to completely finish this ticket by the end of the school year (may 5)
Elliot

@amyjko
Copy link
Collaborator

amyjko commented Apr 15, 2024

Exciting progress, thanks @ElliotRoe.

  • Since we're storing recently used at the project level, yes, versioning this using the pattern is right, and initializing them to an empty list seems reasonable.
  • ProjectView is doing a lot right now; I think when we eventually migrate to Svelte 5 once it's officially released, this will be a lot cleaner, but for now, I'm loathe to add more weight to an already monolithic component. I think centralizing the logic around glyph usage in the search area is fine; there are many other components that encapsulate project updates already using the common global interface for updating projects. We can always refactor later, so this seems reasonable for now.

@ElliotRoe
Copy link
Author

Quick update about my work on this ticket as I'd like to document my work on this sub-issue so far. I've been diving into the scrolling performance issues which is particularly seen when scrolling the "other" category because of it's large size. I've gone through several steps to localize the issue:

  1. Removed the use of the VirtualList component
    • This creates a large CPU usage spike as soon as you select the "other" category then the same performance issues persist as the user scrolls through the glyphs
    • It doesn't seem like the virtual list component is the root of this issue
  2. Only rendered 1 glyph at a time
    • Improves performance, which seems to indicate that complex nature of the Button and Token view components are causing this slow rendering
  3. Removed TokenView component and replaced with raw string of glyph
    • Improves performance, but does not resolve issue
  4. Removed Button & TokenView and replaced with raw string of glyph
    • Good performance at max scroll speed

Conclusion: It seems like the complex nature of the Button and TokenView components are causing the slow down as 100s of components must be rendered over a couple seconds at max scroll speed.

Possible Solutions

Here are some solutions I'll be exploring to fix the issue

  1. Limit scroll speed
    • Issue: Doesn't feel great to limit user action in this way
  2. Pause scrolling like in this example
    • Issue: Does not solve the case of scrolling back up quickly
  3. Paginate the pages
    • Issue: Not in original design, scrolling seems much more intuitive
  4. Lazy Rendering (e.g. render strings when scrolling & full component when not scrolling)
    • Issue: Concerned this could cause accessibility pitfalls, but not completely sure

My current plan is move forward with solution 2 and implement a way to limit scrolling back up as it seems the most simple. Of course, open to suggestions and thoughts for other solutions too. Thanks

@amyjko
Copy link
Collaborator

amyjko commented May 7, 2024

Thanks for digging into the performance issues in detail here; scale is tricky, no matter how zippy Svelte is! (It will be getting zippier with v5, but probably not enough to address this scale).

From a UX perspective, I think following pagination conventions (options 2 and 3) seem most ideal, since they will be familiar. The only alternative would be a raw string glyph instead of interactive component, and then handing events at the container level instead of the glyph level, avoiding Button and TokenView. I'm not opposed to that if you want to tackle that. The challenge there is accessibility: every glyph still needs to be focusable and selectable, so it would probably need to implement something like the layout grid role.

@ElliotRoe
Copy link
Author

Gotcha, that makes sense. I would be happy to tackle creating a lightweight scrollable component (seems super fun to build) that meet those specifications, especially if you feel that it would be useful and reusable in the larger context of the code base.

However, I'm hesitant to continue to add to the list of tasks in this ticket. I think for now I'll implement some pagination options you mentioned above. Then, open a separate ticket detailing the requirements of the light weight scroll components, if needed.

@amyjko
Copy link
Collaborator

amyjko commented May 7, 2024

Pagination sounds reasonable for now. Thank you for the steady work on this!

@amyjko
Copy link
Collaborator

amyjko commented Sep 12, 2024

Just checking in on this PR. Think you'll want to finish it up at some point?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Group Unicode symbols into browsable categories
2 participants